-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix validate_atomic_addr()
#5063
Conversation
crates/runtime/src/libcalls.rs
Outdated
if addr < mem.base as usize { | ||
return Err(TrapCode::HeapOutOfBounds); | ||
} | ||
if addr >= mem.base.add(mem.current_length.load(Ordering::Relaxed)) as *const _ as usize { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
checked_add
?
These intrisnics were added quite some time ago and AFAIK haven't ever really been designed for "ok this is how they should actually work", instead they've just sort of "accidentally" been kept working as the bare-bones ability to run the For example the intrinsics currently do That's an example of what's going on here but I suspect there may be other things. Overall if you're interested to see these intrinsics implemented fully I think it might be best to flesh that all out at once rather than incrementally, especially given the trickiness of implementing them correctly and importance of implementing them correctly. For the technical contents of this PR itself, personally I find it odd that a real address would be passed along here simply to get re-converted into a relative offsets. I personaly think that either the intrinsics shouldn't need the relative offset or they should take the relative offset, rather than being in a weird "go between" as they are now. |
@alexcrichton well, maybe the absolute address makes sense, if you look at a naive futex implementation of notify and wait: |
Yes in that implementation it does look like it makes sense. That's somewhat orthogonal to my point though of this libcall, in my opinion, should either take the relative offset and calculate the real address or it should take the real address. I dont think it makes sense for cranelift code to do the math for the real address only to have the libcall undo the math again. |
crates/runtime/src/libcalls.rs
Outdated
if addr < mem.base as usize { | ||
return Err(TrapCode::HeapOutOfBounds); | ||
} | ||
if addr >= mem.base.add(mem.current_length.load(Ordering::Relaxed)) as *const _ as usize { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this also need to account for access sizes larger than 1? For example a 8 byte access where the address is the last byte of the memory should not be allowed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Misaligned access will trap. This check is done in JIT code:
wasmtime/cranelift/wasm/src/code_translator.rs
Lines 1070 to 1071 in 32a7593
let (_flags, addr) = | |
prepare_atomic_addr(memarg, implied_ty.bytes(), builder, state, environ)?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would necessitate all users of this function to trap on misalignment. Currently all callers of this function seem to immediately be followed by an unimplemented trap. Once they are implemented it is easy to miss this requirement as misaligned atomic accesses in rust are not guaranteed to panic.
41c2c58
to
e55425d
Compare
Corrected the check. Now checks start and end address of the atomic. |
Apparently `addr` is a pointer to real memory. Signed-off-by: Harald Hoyer <[email protected]>
e55425d
to
fd16786
Compare
@abrown, yes, closing |
Apparently
addr
is a pointer to real memory.Signed-off-by: Harald Hoyer [email protected]